Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

Copy link
Contributor Author

MasterPtato commented Jan 12, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 12, 2026

PR Review: Clean up errors

Summary

This PR consolidates error definitions by:

  1. Moving the Datacenter::NotFound error from api-public to api-util for better code reuse
  2. Removing unused guard errors (actor_destroyed, actor_not_found)
  3. Renaming test error artifacts to use test group with combined group+code names
  4. Improving error handling in guard-core tests

Code Quality ✅

Good practices observed:

  • Proper error consolidation - moving Datacenter::NotFound to api-util follows DRY principles since it's used by multiple packages
  • Making errors module public (pub mod errors;) enables proper reuse across packages
  • Test error artifacts correctly renamed to avoid confusion with production errors
  • Clean removal of unused error definitions

Implementation Review

1. Error Migration (api-util/api-public) ✅

The Datacenter::NotFound error migration is well-executed:

  • Moved from api-public/src/errors.rs to api-util/src/errors.rs
  • Updated all references in:
    • packages/api-public/src/actors/utils.rs:144
    • packages/api-public/src/runner_configs/upsert.rs:99
  • The error is used in api-util/src/lib.rs (lines 36, 74), justifying its location

2. Unused Error Cleanup ✅

Removal of unused guard errors is appropriate:

  • guard.actor_destroyed.json
  • guard.actor_not_found.json

These appear to be legitimately unused. If they were referenced anywhere, the build would fail.

3. Test Error Artifacts ✅

Renaming test errors improves clarity:

  • api.rate_limited.jsontest.api_rate_limited.json
  • namespace.invalid_name.jsontest.namespace_invalid_name.json

This prevents confusion between test fixtures and production error definitions. The corresponding test updates in engine/packages/error/tests/basic.rs maintain consistency.

4. Guard Test Improvements ✅

The changes in guard-core/tests/streaming_response.rs improve code quality:

  • Early return pattern (return Ok(...)) is clearer than assigning to a variable
  • Using anyhow::anyhow!("bad path") instead of bail!("bad path") is more idiomatic when not using the bail! macro's shorthand benefits
  • However, the original code with bail! was also fine - this is a minor style preference

Minor consideration: The change from bail! to anyhow::anyhow! removes the use anyhow::bail; import. While both work correctly, bail! is typically preferred for simple error messages as it's more concise. This is a very minor point and doesn't affect functionality.

Potential Issues

No issues found. All changes are backward compatible and properly implemented.

Performance Considerations ✅

No performance impact. This is a refactoring/cleanup PR with no runtime changes.

Security Considerations ✅

No security concerns. Error handling remains unchanged functionally.

Test Coverage ✅

Test coverage is maintained:

  • Test files updated to reflect renamed error artifacts
  • No functionality changes that would require new tests
  • Existing tests validate the error system works correctly

Adherence to CLAUDE.md Guidelines ✅

  • ✅ Error definitions follow the documented pattern with #[derive(RivetError)]
  • ✅ Error groups properly categorized ("datacenter", "test", "validation")
  • ✅ Proper use of workspace dependencies
  • ✅ No backwards-compatibility hacks

Recommendations

All changes look good! This is a clean refactoring PR that:

  1. Reduces code duplication
  2. Improves error organization
  3. Removes unused code
  4. Clarifies test vs. production errors

The PR is ready to merge. No action items required.

Minor Note (optional, not blocking)

In guard-core/tests/streaming_response.rs:256, the switch from bail! to anyhow::anyhow! is functionally equivalent. If you want to maintain consistency with other error handling in the codebase, bail!("bad path") is slightly more idiomatic for simple error messages, but the current implementation is perfectly fine.


Verdict: LGTM ✅ - Well-executed cleanup PR with no issues.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 12, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3846

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3846

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3846

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3846

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3846

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3846

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3846

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3846

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3846

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3846

commit: 520df1b

@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 66f6892 to 485634a Compare January 13, 2026 00:27
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch 2 times, most recently from 363b7a5 to 3f06e1b Compare January 13, 2026 00:44
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 485634a to 964ae0f Compare January 13, 2026 00:44
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 3f06e1b to 8cd75d7 Compare January 13, 2026 01:01
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 964ae0f to f3c17ef Compare January 13, 2026 01:02
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 8cd75d7 to 75ce8e5 Compare January 13, 2026 01:29
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from f3c17ef to b5999a3 Compare January 13, 2026 01:29
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 75ce8e5 to 50ba096 Compare January 14, 2026 02:05
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch 2 times, most recently from 27fc351 to 77119ca Compare January 14, 2026 02:53
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 50ba096 to b9a9c50 Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 77119ca to 1fe301c Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from b9a9c50 to 9a4ab5a Compare January 14, 2026 19:47
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch 2 times, most recently from 830c32d to dfbb13a Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 9a4ab5a to 33e8961 Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 33e8961 to 633544a Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from dfbb13a to 81663b7 Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 633544a to e073155 Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch 2 times, most recently from 260900c to a4f42a6 Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from e073155 to e96badf Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from a4f42a6 to 520df1b Compare January 14, 2026 23:39
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from e96badf to 448ef1f Compare January 14, 2026 23:39
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 14, 2026

Merge activity

  • Jan 14, 11:40 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
  • Jan 14, 11:41 PM UTC: CI is running for this pull request on a draft pull request (#3908) due to your merge queue CI optimization settings.
  • Jan 14, 11:42 PM UTC: Merged by the Graphite merge queue via draft PR: #3908.

graphite-app bot pushed a commit that referenced this pull request Jan 14, 2026
@graphite-app graphite-app bot closed this Jan 14, 2026
@graphite-app graphite-app bot deleted the 01-12-chore_clean_up_errors branch January 14, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants